Add support for GKE Accelerator Network Profile#15474
Add support for GKE Accelerator Network Profile#15474melinath merged 48 commits intoGoogleCloudPlatform:mainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
PR Concerns & JustificationsThis PR introduces support for the GKE Accelerator Network Profile (ANP) feature, for the GA release. There are two key changes that require special attention: 1. Change:
2. Change: Alpha API Version Guard for
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
melinath
left a comment
There was a problem hiding this comment.
It looks like this is functioning correctly to me. In particular, when I remove the additional_node_network_configs block, it does trigger replacement. I just have a couple small changes to request to how tests are structured.
| }, | ||
| // Step 2: Remove Manual Config (Expect Replacement) | ||
| { | ||
| Config: testAccContainerCluster_nodePool_acceleratorNetworkProfile_basic(clusterName), |
There was a problem hiding this comment.
Here & elsewhere: since it's important that the replacement is happening, it would be good to check that explicitly. (It's working now, but this will help ensure it doesn't get broken in the future.)
| Config: testAccContainerCluster_nodePool_acceleratorNetworkProfile_basic(clusterName), | |
| Config: testAccContainerCluster_nodePool_acceleratorNetworkProfile_basic(clusterName), | |
| ConfigPlanChecks: resource.ConfigPlanChecks{ | |
| PreApply: []plancheck.PlanCheck{ | |
| plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionReplace), | |
| }, | |
| }, |
| resource.TestCheckResourceAttr(resourceName, "node_pool.0.network_config.0.accelerator_network_profile", "auto"), | ||
| ), | ||
| }, | ||
| // Step 4: Import Verify |
There was a problem hiding this comment.
Rather than having one separate import verify step, there should be an import verify step after every config step.
There was a problem hiding this comment.
Got it, changed the tests. And also thank you for modifying the tests syntax as well!
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_node_pool_test.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_node_pool_test.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_node_pool_test.go.tmpl
Show resolved
Hide resolved
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing doc report (experimental)The following resources have fields missing in documents.
|
Tests analyticsTotal tests: 278 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
melinath
left a comment
There was a problem hiding this comment.
Okay, approving and merging!
9c56fcc
|
@ellenjzh looks like a bunch of tests failed because of this (e.g., hashicorp/terraform-provider-google#25898 and similar). Was this submitted too soon? Not sure if this was ready to be usable yet. |
I see the error here, GA tests didn't pass because I didn't add the GA version guards. The feature is only available in Beta. I will add the GA version guards to the tests now. |
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…15474) Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…15474) Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…15474) Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.